Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compute values for rational magnitude powers #601

Merged
merged 6 commits into from
Jul 30, 2024

Conversation

chiphogg
Copy link
Collaborator

@chiphogg chiphogg commented Jul 24, 2024

Since this will only ever be done at compile time (as guaranteed by
using consteval), we can afford to prioritize precision over speed. To
compute an Nth root, we simply do a binary search over representable
floating point numbers, looking for the number whose Nth power most
closely matches the original number.

Fixes #474. We have included a test case reproducing the original
problem exactly. All tests use "within 4 ULPs" as the criterion, which
is (I believe) equivalent to the googletest EXPECT_DOUBLE_EQ
criterion.

Since this will only ever be done at compile time (as guaranteed by
using `consteval`), we can afford to prioritize precision over speed.
To compute an Nth root, we simply do a binary search over representable
floating point numbers, looking for the number whose Nth power most
closely matches the original number.

Fixes mpusz#494.  We have included a test case reproducing the original
problem exactly.  All tests use "within 4 ULPs" as the criterion, which
is (I believe) equivalent to the googletest `EXPECT_DOUBLE_EQ`
criterion.
@chiphogg chiphogg requested a review from mpusz July 24, 2024 14:35
test/static/quantity_test.cpp Outdated Show resolved Hide resolved
@mpusz
Copy link
Owner

mpusz commented Jul 24, 2024

"Formatting CI" fails. Please run pre-commit according to the instructions in https://github.com/mpusz/mp-units/blob/master/CONTRIBUTING.md#unified-code-formatting.

@mpusz
Copy link
Owner

mpusz commented Jul 24, 2024

math.h is not constexpr in libc++. Please maybe use runtime tests for that or provide a compilation condition similar to

#if __cpp_lib_constexpr_cmath || MP_UNITS_COMP_GCC

@chiphogg chiphogg requested a review from mpusz July 27, 2024 23:34
@mpusz
Copy link
Owner

mpusz commented Jul 29, 2024

It seems that the libc++-18 issues are fixed now, and a few problems have surfaced in our code. I fixed all of them today. Please rebase to master and fix the remaining issues in your code. There are at least two more issues to be fixed:

@chiphogg
Copy link
Collaborator Author

Apple clang does not like this linkage:

image

It's weird, because the undefined symbol is mp_units::detail::expr_multiply<mp_units::derived_unit, mp_units::one, mp_units::detail::type_list_of_unit_less, mp_units::si::metre, mp_units::si::metre>(mp_units::si::metre, mp_units::si::metre), and expr_multiply comes from expression_template.h, which is in mp_units::core. And unit_test_runtime is already linked to mp_units::mp_units, which itself depends on mp_units::core. So as far as I can tell, we should be good!

Maybe the solution is more obvious to you.

@mpusz
Copy link
Owner

mpusz commented Jul 30, 2024

This was also the case for clang-16:
https://github.com/mpusz/mp-units/actions/runs/10154113253/job/28078706674?pr=601

Hopefully, now it will be OK.

@chiphogg
Copy link
Collaborator Author

Really glad I asked for help instead of trying to solve it on my own --- I would never have come up with that!

@mpusz
Copy link
Owner

mpusz commented Jul 30, 2024

Yeah, I had a few issues with clang-16 already 😉

@chiphogg
Copy link
Collaborator Author

I had to manually restart each of 3 failed jobs. The jobs gave no reason for their failure, and all of them passed on retry. In any case, we're at a 100% pass rate right now!

Copy link
Owner

@mpusz mpusz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @chiphogg!

@mpusz mpusz merged commit ab51b47 into mpusz:master Jul 30, 2024
299 checks passed
@chiphogg chiphogg deleted the chiphogg/sqrt-mag#474 branch August 4, 2024 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Odd behavior when taking a square root
2 participants